Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix diff functionality #558

Closed
wants to merge 3 commits into from

Conversation

cameronklein
Copy link

@cameronklein cameronklein commented Aug 11, 2023

Description

Reverts to the original diff functionality

Issue: #557

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

@docs-page
Copy link

docs-page bot commented Aug 11, 2023

To view this pull requests documentation preview, visit the following URL:

melos.invertase.dev/~558

Documentation is deployed and generated using docs.page.

@CLAassistant
Copy link

CLAassistant commented Aug 11, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ spydon
✅ Salakar
❌ cameron-klein
You have signed the CLA already but the status is still pending? Let us recheck it.

@cameronklein
Copy link
Author

Thanks! I signed the agreement but it doesn't seem to be updating. I've clicked the recheck link a few times but no luck. It's also not letting me sign it again.

Screenshot 2023-08-11 at 1 10 27 PM

@Him-Sharma
Copy link

Thanks for fixing this. Do you know when can we merge this? this is impacting our workflows as well.

@Salakar
Copy link
Member

Salakar commented Nov 6, 2023

Is there any implications on reversing this? I understand the original change was done mainly for cleanup but not sure the reasoning of switching the diff logic.

Maybe there is there an interim way we can support both for now that way we ship it faster and worry about reversal implications later?

@spydon
Copy link
Collaborator

spydon commented Feb 4, 2024

@cameronklein did you see the questions from Salakar? :)

@cameronklein
Copy link
Author

It does make sense to support both but I personally don't have the bandwidth. We've forked the repo and made the change for our use case. I'll keep the issue (#557) open for anyone that does have more time!

@spydon
Copy link
Collaborator

spydon commented Mar 14, 2024

It does make sense to support both but I personally don't have the bandwidth. We've forked the repo and made the change for our use case. I'll keep the issue (#557) open for anyone that does have more time!

Maybe it is worth opening up and merging this anyways, is there any usecase for supporting both? Isn't this way just "more correct"?

@xsahil03x
Copy link
Contributor

Yes, I agree. @cameron-klein can you please re-open this PR and we can hopefully get it merged this time? Thanks

@spydon
Copy link
Collaborator

spydon commented Mar 15, 2024

It would be great to have your input here @blaugold, because I don't fully get it there would be any downsides.

@xsahil03x
Copy link
Contributor

Hey @blaugold can you please take a look at this today? Thanks

@xsahil03x
Copy link
Contributor

Hey @spydon, what do you suggest we should do next?

@spydon
Copy link
Collaborator

spydon commented Mar 20, 2024

Hey @spydon, what do you suggest we should do next?

Could you open up a new PR with the only changes that are needed from this PR and I could get some other people to review it too? And I'll do some more research on what these changes would entail, so that I can review it better.

@blaugold
Copy link
Collaborator

Sorry for the delay. I'm taking a look at it now.

@blaugold
Copy link
Collaborator

If I remember correctly, I moved from git diff to git log for detecting changes in a package only for simplifying things and not for any functionality.

These changes look to me like they only improve the correctness of --diff and I can't see any functionality being taken away. I think it makes sense to merge this PR.

@spydon spydon mentioned this pull request Mar 21, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants